Actually retract clashing synthetic apply/unapply #5846
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjrd
approved these changes
Apr 12, 2017
|
(Forgot the flags files of course :-)) |
lrytz
reviewed
Apr 12, 2017
| // (1) If we're already in the loop, set the IS_ERROR flag and trigger the condition | ||
| // `sym.initialize.hasFlag(IS_ERROR)` in typedStats::addSynthetics, | ||
| // (2) Or, if we are not yet in the addSynthetics loop (and we're not going to emit an error anyway), | ||
| // we unlink the symbol from its scope. | ||
| sym setFlag IS_ERROR |
There was a problem hiding this comment.
- add
companionContext.unit.synthetics -= sym?
Also make this whole retraction of apply/unapply in case of a clashing user-defined member conditional on `-Xsource:2.12`. It turns out, as explained by lrytz, that the retraction mechanism was fragile because it relied on the order in which completers are run. We now cover both the case that: - the completer was run, the `IS_ERROR` flag was set, and the symbol was unlinked from its scope before `addSynthetics` in `typedStat` iterates over the scope (since the symbol is already unlinked, the tree is not added, irrespective of its flags). For this case, we also remove the symbol from the synthetics in its unit (for cleanliness). - the completer is triggered during the iteration in `addSynthetics`, which needs the check for the `IS_ERROR` flag during the iteration. Before, the completer just unlinked the symbol and set the IS_ERROR flag, and I assumed the typer dropped a synthetic tree with a symbol with that flag, because the tree was not shown in -Xprint output. In reality, the completer just always happened to run before the addSynthetics loop and unlinked the symbol from its scope in the test cases I came up with (including the 2.11 community build). Thankfully, the 2.12 community build caught my mistake, and lrytz provided a good analysis and review. Fix scala/bug#10261
densh
approved these changes
Apr 13, 2017
lrytz
approved these changes
Apr 13, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also make this whole retraction of apply/unapply in case of a
clashing user-defined member conditional on
-Xsource:2.12.It turns out, as explained by lrytz, that the retraction mechanism
was fragile because it relied on the order in which completers are run.
We now cover both the case that:
the completer was run, the
IS_ERRORflag was set, and thesymbol was unlinked from its scope before
addSyntheticsin
typedStatiterates over the scope (since the symbol isalready unlinked, the tree is not added, irrespective of its flags).
For this case, we also remove the symbol from the synthetics in
its unit (for cleanliness).
the completer is triggered during the iteration in
addSynthetics,which needs the check for the
IS_ERRORflag during the iteration.Before, the completer just unlinked the symbol and set the IS_ERROR flag,
and I assumed the typer dropped a synthetic tree with a symbol with
that flag, because the tree was not shown in -Xprint output.
In reality, the completer just always happened to run before the
addSynthetics loop and unlinked the symbol from its scope in the
test cases I came up with (including the 2.11 community build).
Thankfully, the 2.12 community build caught my mistake, and lrytz provided
a good analysis and review.
Fix scala/bug#10261